Add basic multi-partition GroupBy support to cuDF-Polars
#17503
Conversation
rjzamora
added
feature request
|
Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually. Contributors can view more details about this message here. |
github-actions
bot
added
Python
|
/ok to test |
| # Check that we are grouping on element-wise | ||
| # keys (is this already guaranteed?) | ||
| for ne in ir.keys: | ||
| if not isinstance(ne.value, Col): # pragma: no cover | ||
| return _single_fallback(ir, children, partition_info) |
There was a problem hiding this comment.
What do you mean by elementwise keys? It's certainly not the case that we always group on columns. But I think it is the case that the group keys (if expressions) are trivially elementwise (e.g. a + b as a key is fine, but a.unique() or a.sort() is not)
There was a problem hiding this comment.
Right. I'm being extra cautious by requiring the keys to be Col. This comment is essentially asking: "can we drop this check altogether? ie. Will the keys always be element-wise?
There was a problem hiding this comment.
Opened pola-rs/polars#20152 as well
|
This PR is pretty-much "ready" - I don't think it makes sense to build more groupby logic directly on top of this. It would be much better to revise the underlying |
rjzamora
added
3 - Ready for Review
|
@wence- I'm marking this as "ready for review". As we discussed offline, this will need to be revised for general support. However, the current design should be sufficient for basic TPCH support/testing. |
| .items() | ||
| if c in groupby_key_columns | ||
| } | ||
| if cardinality_factor: |
There was a problem hiding this comment.
This "cardinality factor" logic is new, and can be pulled out of the PR if necessary. However, we do need a mechanism to trigger shuffle-based groupby aggregations in practice.
|
@wence- Any action items left for this PR? |
| ir.schema, | ||
| ir.keys, | ||
| piecewise_exprs, | ||
| ir.maintain_order, |
There was a problem hiding this comment.
I don't think we can support maintain_order == True (at least easily). So perhaps we should raise in that case.
There was a problem hiding this comment.
I think we support it for the tree reduction, but not for a shuffle-based reduction, right? The tree-reduction tasks should be ordered appropriately.
|
Thanks for the review @wence- ! I tried addressing your comments - but I suppose I had to rewrite most of the code in the process. |
| ir.schema, | ||
| ir.keys, | ||
| piecewise_exprs, | ||
| ir.maintain_order, |
rjzamora
added
5 - Ready to Merge
|
/merge |
Description
Adds multi-partition support for simple
GroupByaggregations (following the same design as #17441)Checklist